-
Notifications
You must be signed in to change notification settings - Fork 70
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve user profile card handling #3103
Improve user profile card handling #3103
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a few comments.
@@ -93,10 +91,10 @@ public void setUserProfile(@Nullable UserProfile userProfile) { | |||
if (userProfile != null) { | |||
userName.setText(userProfile.getUserName()); | |||
} | |||
configureOpenProfileCard(userProfile); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think adding this by default makes a great UX.
In particular, when the user profile display or icon are used in tables or dropdowns, clicking on the user icon will do two things: open the popup and select that row/menu. If the user's intention was to the latter, then it is annoying and confusing. Therefore, I would stick to adding it in a case by case scenario. See: #3041 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm... i got confused that in some cases i could not open it like in offer list and some other places.
We could invert the handling by using a method to not open a popup if not wanted, that would reduce the cases where we need to deal with it. But not too strong opinion on that. We can also revert it, but IMO it was missing in some places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which cases do you have in mind to not apply it?
@@ -71,47 +83,59 @@ public ProfileCardDetailsView(ProfileCardDetailsModel model, | |||
lastUserActivityLabel = new Label(); | |||
HBox lastUserActivityBox = createAndGetTitleAndDetailsBox("user.profileCard.details.lastUserActivity", lastUserActivityLabel); | |||
|
|||
// Version | |||
versionLabel = new Label(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this was useful to have, especially for moderators/mediators.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. As discussed in matrix, we can change the overview/details content. i will make a commit soon
super(new VBox(), model, controller); | ||
|
||
// Statement | ||
statementLabel = new Label(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would leave this field here instead of details, and show this as first page if any of the two is set.
Then we can revert to the original rectangular dimensions that are more characteristic of a "card", hence the profile card name.
…te chat now. Move handling for opening user profile card to UserProfileIcon and UserProfileDisplay.
Replace version with statement (terms will become tab if present) remove unneeded properties and bindings
Show tab only if present Use existing i18n strings for statement and terms to avoid diverging translations
04f14e7
to
bf709b7
Compare
d9b2315
to
9823dd8
Compare
No description provided.